Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix documentation rendering #965

Merged
merged 67 commits into from
Sep 24, 2024
Merged

Fix documentation rendering #965

merged 67 commits into from
Sep 24, 2024

Conversation

ElliotFriend
Copy link
Contributor

I'm trying to get all the JSDoc rendering to work now that the SDK has had a chance to settle a bit with its inclusion of the Soroban functionality.

Refs: #920

Copy link

github-actions bot commented May 14, 2024

Size Change: +288 kB (+0.6%)

Total Size: 48.4 MB

Filename Size Change
dist/stellar-sdk-minimal.js 6.66 MB +47 kB (+0.71%)
dist/stellar-sdk-minimal.min.js 5.21 MB +25 kB (+0.48%)
dist/stellar-sdk-no-axios.js 6.66 MB +47 kB (+0.71%)
dist/stellar-sdk-no-axios.min.js 5.21 MB +25 kB (+0.48%)
dist/stellar-sdk-no-eventsource.js 6.92 MB +47 kB (+0.68%)
dist/stellar-sdk-no-eventsource.min.js 5.42 MB +25 kB (+0.46%)
dist/stellar-sdk.js 6.92 MB +47 kB (+0.68%)
dist/stellar-sdk.min.js 5.42 MB +25 kB (+0.46%)

compressed-size-action

@Shaptic
Copy link
Contributor

Shaptic commented May 14, 2024

Yo this is absolutely Herculean, you freakin' rock

Since the comments clearly say "don't make this class yourself,"
it might make sense to mark them as `@private` so they don't show
up in the documentation.

The doc pages for those classes can still be visited, but they
don't show up in the sidebar to the left, unless you specifically
navigate to one of them. (Or, if you click a link for what the
return type on the `Horizon.Server.effects()` method, for example
I don't _think_ they're accessible by SDK users, so I think it
makes sense to not render them in the docs?
@Shaptic Shaptic mentioned this pull request Jun 18, 2024
@ElliotFriend
Copy link
Contributor Author

@Shaptic I think this is ready to go! Thanks for checking it out!

@ElliotFriend ElliotFriend marked this pull request as ready for review September 20, 2024 17:50
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rocks my socks off 🎸 AMAZING work 👏

Can you make sure you run yarn _prettier one more time? Then I'll merge

src/horizon/server.ts Show resolved Hide resolved
@ElliotFriend
Copy link
Contributor Author

@Shaptic when I run yarn _prettier, there are no resulting changes. Which seems... unlikely lol. If i'm reading the underlying command right, it seems like it's actually only running on the /tests directory?

"_prettier": "prettier --ignore-path config/.prettierignore --write './test/**/*.js'"

If I run that replacing ./test/**/*.js with ./src/**/*.ts, there's plenty of fixes to go around, though lol. Want me to commit that?

@ElliotFriend
Copy link
Contributor Author

ElliotFriend commented Sep 20, 2024

I ended up just committing all those changes in a single commit, so it'll be easy enough to revert if you want to: 841f7c9

Edit: I did a bit of digging, and it looks like commit 21bd09 from #860 is when the prettier directory changed from ./**/*.js to ./tests/**/*.js. Don't know why the change was made, but at least i found it 🤣

Another Edit: I built and previewed the docs after the prettier changes, and everything looks the same as before. Still good to go, I think

@Shaptic
Copy link
Contributor

Shaptic commented Sep 23, 2024

Ah yeah we def don't want that; I keep forgetting it's only for tests. I'd like to do it for ts files but not in this commit because it'll bork a BUNCH of other PRs.

@Shaptic Shaptic merged commit afb7814 into master Sep 24, 2024
10 checks passed
@Shaptic Shaptic deleted the 920-documentation-rendering branch September 24, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants